Skip to content

removed project warnings #683

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged

Conversation

albertopeam
Copy link
Contributor

@albertopeam albertopeam commented Nov 25, 2022

The warnings this PR fixes:

Screenshot 2022-11-25 at 20 45 30

@albertopeam
Copy link
Contributor Author

albertopeam commented Nov 26, 2022

@JeneaVranceanu thanks for the feedback, I have done the tasks from your feedback :)

I have a doubt about swiftLint, how are you using it? I have run it via command line but I don´t know if it is the way of working or not :)
swiftLint output: Done linting! Found 1940 violations, 125 serious in 187 files.

@albertopeam
Copy link
Contributor Author

@JeneaVranceanu did I miss something? :)

@yaroslavyaroslav
Copy link
Collaborator

I have a doubt about swiftLint, how are you using it? I have run it via command line but I don´t know if it is the way of working or not

We've actually doesn't apply it to the project yet, so this huge list of issues that you reference to is the real one for now.

And about your PR, here's the deal: @JeneaVranceanu is now fighting with some important merge that brings 3.1.0 release in the game, I suppose it would take a week or two from now until he finished it.

And as I suppose right after that we'll come back to your PR which is indeed important as well, but as I see no so complicated in terms of possibilities of breaking something of conflicts would be appeared whilst merging.

@albertopeam
Copy link
Contributor Author

Thanks for the clarification @yaroslavyaroslav :)
I will be waiting for 3.1.0

@janndriessen
Copy link
Collaborator

@albertopeam please resolve conflicts and we will review again.

Same for #688

@albertopeam albertopeam force-pushed the feature/remove-warnings branch from 282ef52 to 1381c98 Compare December 29, 2022 19:08
@albertopeam
Copy link
Contributor Author

@janndriessen ready to review

janndriessen
janndriessen previously approved these changes Dec 30, 2022
Copy link
Collaborator

@janndriessen janndriessen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! It was already well reviewed before by @JeneaVranceanu too. Last changes were just about merge conflicts. ✅

Copy link
Collaborator

@yaroslavyaroslav yaroslavyaroslav left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm back and within next week highly likely will review all prs.

Comment on lines 113 to 116
extension Array where Element == Any? {
func toAnyObject() -> [AnyObject] {
self.map { $0 as AnyObject }
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This could fail, since Any is more broad that AnyObject is, so the casting could fail.

Copy link
Collaborator

@JeneaVranceanu JeneaVranceanu Jan 10, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I do not see how this can cause problems. @yaroslavyaroslav Could you please give more explanation/examples?

It's said that all classes implicitly conform to AnyObject but we're able to cast structs as well so maybe that part about structs (or to be specific value types is confusing).


Did some research and found that casting value types to AnyObject produces a wrapper of type __SwiftValue. And here is the __SwiftValue. Look at the docs on line 13.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lol, why did they do that? Like now there's completely no difference between Any and AnyObject isn't it? If so, i agree that this would not fall, but i still suggest to make that extension more consistent, using either Any or AnyObject in whole method.

Copy link
Collaborator

@JeneaVranceanu JeneaVranceanu Jan 10, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is still some difference when it comes to casting back to the original type. I cannot explain that in detail but for example if we cast UInt64(1) to AnyObject and try to use it as if let bool = uint64AsAnyObject as? Bool it will successfully cast to Bool. But if you cast it to Any instead it will cast back only as UInt64.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

but for example if we cast UInt64(1) to AnyObject and try to use it as if let bool = uint64AsAnyObject as? Bool it will successfully cast to Bool. But if you cast it to Any instead it will cast back only as UInt64.

Actually I'd consider this as a bug of those thing that is casting to AnyObject for Objective-C environment. But I don't sure though.

// MARK: - Conversion

/// Transforms `[Any?]` into `[AnyObject]`
extension Array where Element == Any? {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. This extension doesn't apply to [Any] but it should. As a solution where clause can be removed;
  2. I'd suggest renaming the function to toAnyObjectArray;
  3. Why self.map { ... } and not just as [AnyObject]?

This is the suggested update:

/// Transforms into `[AnyObject]`
extension Array {
    func toAnyObjectArray() -> [AnyObject] {
        self as [AnyObject]
    }
}

@JeneaVranceanu JeneaVranceanu merged commit 496a9fd into web3swift-team:develop Jan 13, 2023
@albertopeam albertopeam deleted the feature/remove-warnings branch January 16, 2023 20:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants